fix(expandable components): Add/update aria attributes to expandable components #3054
Conversation
|
View your CI Pipeline Execution ↗ for commit 8a23092. ☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit c1991ca.
☁️ Nx Cloud last updated this comment at |
| <FlexBox> | ||
| <DisclosureButtonWrapper | ||
| aria-expanded={isExpanded} | ||
| aria-controls={isExpanded ? ariaControlsId : undefined} |
There was a problem hiding this comment.
should the id be on the button regardless of the expanded state?
There was a problem hiding this comment.
Woops, will update the testing instructions with the SR info, thanks!
—
re: id
tl;dr: an axe test was failing when, I think, aria-controls was trying to reference an id on an element that wasn't rendered yet. Adding this ternary solved the issue, but wondering if all these aria-controls should follow the pattern.
mini-story version: I didn't realize that Disclosure didn't have aria-expanded or aria-controls :\ so this is a new addition.
After I finished up with fixing some other failing mono tests, I saw that there was a failure on a Disclosure because of an "invalid value for aria-controls" https://github.com/codecademy-engineering/mono/actions/runs/14248719474/job/39936761982
Which I thought maybe that's cause I was using useId() and that values weren't readable to a person. So I swapped ALL of the useId instances with something more readable. And it was still failing lol (and I also had to fix a typo in the new id too x.x) So I read a bit more into it and saw this post: https://github.com/codecademy-engineering/mono/actions/runs/14248719474/job/39936761982 that mentioned that the element with the id should be on the page.
Then I updated Disclosure's aria-controls to this ternary and it fixed the issue. But I was hesitant about adding this logic to all the elements.
There was a problem hiding this comment.
A couples questions and things I'm not seeing in storybook but mostly looking good!
- not seeing aria-controls on drawer/flyout when the component is collapsed
- should
Disclosurehavearia-controls? the PR desc says no but testing instructions says yes - I don't see the id on the info tip, but possible i'm missing it or it needs to be added to the story
packages/styleguide/src/lib/Molecules/Flyout/Flyout.stories.tsx
Outdated
Show resolved
Hide resolved
Adds bi-directional option for multiview modals, refactors the CTAs in multiview modals, and adds danger variant. Adds unit tests and additional stories.
- @Codecademy/gamut@62.0.0 - @codecademy/[email protected] - @codecademy/[email protected]
Applying changes to the Modal and Dialog components per feedback from a11y team. Also adding a zIndex
- @Codecademy/gamut@62.0.1 - @codecademy/[email protected] - @codecademy/[email protected]
| <ExpandControl | ||
| expanded={isExpanded} | ||
| onExpand={() => setExpanded(!isExpanded)} | ||
| disabled={false} | ||
| /> |
There was a problem hiding this comment.
this button used to have yellow text and now its grey. probably fine since this is just the storybook example but wanted to note
There was a problem hiding this comment.
ya, thanks Amy — I should've left a comment.
That seems intentional based on what the original component, I'll ask Stacey just in case :)
There was a problem hiding this comment.
Gonna update to secondary to match the textButton styling :) thanks again~
📬Published Alpha Packages:@codecademy/[email protected] |
|
🚀 Styleguide deploy preview ready! |
Overview
Components that expand other components should have an
aria-expandsattribute that denotes if an expandable component is expanded or not.When using Voiceover, an element with an
aria-expandedattribute, should announce if the element is "expanded" or "collapsed".expanded example:
collapsed example:
This PR adds the
aria-expandedattribute toDisclosureandInfoTipIt also updates guidance for
Drawer+FlyoutAlso updates
ListRowto useExpandControl.Note: previously this PR also included
aria-controlsimplementation, but that has moved to: #3069 and tabled for the time beingPR Checklist
Testing Instructions
aria-expandedattributearia-expandedis now updatedPR Links and Envs